Skip to content

Conversation

@scottheng96
Copy link
Contributor

@scottheng96 scottheng96 commented Sep 26, 2025

Problem

MRF decryption doesn't support decryption of verifiedContent. This is needed to support Singpass integrations with MRF as Ndi data is stored in verifiedContent, separate from encryptedContent

Ndi data mainly refers to

  • uinFin number
  • userInfo
    and varies depending on the Singpass form auth type used.

This ndi data is needed for example to know if the submitter is authorised to submit a form - eg, nric whitelisting.

As part of MRF singpass support, some changes are made:

  1. verifiedContent is to be encrypted using the submissionPublicKey (tied to the step) instead of the form public key.
    Why?
    There are some pros to using form public key for encryption, as this means that subsequent steps cannot decrypt and see the submitter id content stored in verified content (1).
    However, to support multi step singpass for MRF, this submitter id content is needed - hence form public key cannot be used.

Hence, submissionPublicKey is used at the expense of (1), where subsequent steps (and submitters) may be able to decrypt the verified content

  1. verifiedContent is to be encrypted in code separate from the main form responses (ie, encryptedContent).
    This is done as the previous storage mode is implemented the same way.
    First, the form responses are encrypted with the form public key (encryptedContent).
    Then, the verified content are encrypted with the form public key (verifiedContent).
    These 2 items are separately generated and encrypted are then added to a common object and saved into the same database document.

For MRF:
a) First, the form responses will be encrypted with the submissionPublicKey (per step) using v3's encrypt. Generating a new submissionPublicKey.
b) Then, the verified content will be separately encrypted with the form submissionPublicKey. For each step, the verifiedContent will be decrypted using the previous submissionPublicKey and then new info for that step will be appended before being re-encrypting using the new submissionPublicKey.

While it is possible to combine step b) and a) together, by expanding v3's encrypt to support encryption of verifiedContent as well. For now, a decision has been made to keep it separate to follow the implementation of v1 more closely.

Closes FRM-2156

Solution

  1. Add verifiedContent to input types & verified to output type for cryptoV3 functions
  2. If verifiedContent is present, decrypt verifiedContent with submissionSecretKey and throw errors if unsuccessful
  3. If verifiedContent not present, skip decryption and return only encryptedContent (backwards compatible)

Before & After Screenshots

Screenshot showing accurate decryption of a MRF submission data (see console & note successful decrypted verifiedContent

image

Dependency Updates

The coveralls 3.1.1 package was causing a security vulnerability. It was preventing a transient package form-data from being upgraded (was 2.3.3), whereby this older version was using an unsafe Math.random() to choose boundaries, which can be predictable by an attacker given enough sequences of outputs.

Updated sdk to use coveralls-next which is a fork of coveralls (no longer maintained). formsh-sdk is bumped to v4.0.4.

@linear
Copy link

linear bot commented Sep 26, 2025


return returnedObject
} catch (err) {
return null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: in crypto.ts,

    } catch (err) {
      // Should only throw if MissingPublicKeyError.
      // This library should be able to be used to encrypt and decrypt content
      // if the content does not contain verified fields.
      if (err instanceof MissingPublicKeyError) {
        throw err
      }
      return null
    }

The missing public key error is thrown to the client - should we perhaps be doing the same thing for cryptov3?

Copy link
Contributor Author

@scottheng96 scottheng96 Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to keep it consistent between the versions. I'll throw the error from decryptFromSubmissionKey and to decrypt as well.

Tbh We don't handle these errors in our code, where we only check for null and return another error else. But that's for later when we update app implementation


// Act
const ciphertext = crypto.encrypt(plaintext, publicKey)
const verifiedText = cryptoV1.encrypt(plainVerifiedText, ciphertext.submissionPublicKey, signingSecretKey)
Copy link

@kevin9foong kevin9foong Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Could we perhaps leave a comment about the rationale behind why cryptoV1 is used for decrypting verifiedContent here?
It might be a little confusing as to why cryptoV1 is being used in the cryptoV3 test file for new engs.

Perhaps something explaining the below comment (but probably more concise).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question - confirm if this is accurate:
For storage mode forms, the verifiedContent is added outside of the main encryptedContent.

For MRF, this verifiedContent is not yet implemented.
Thinking about its implementation and extensibility to MRF singpass for multiple steps. (crucial since changes to SDK usually can be added but hard to be taken away).

For singpass with MRF on the FormSG app side, my understanding is that since it is only for 1st step - we can use cryptoV1 to encrypt with the current step's submission public key.
For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key.

However, if we use cryptoV1 for verified content and use the submission steps's latest submission key - would admins still be able to decrypt the verified content with the form private key using decrypt in cryptoV3?

  • yes, since can pass in {encryptedSubmissionSecretKey, ...verifiedContent} as the DecryptParams and the same encryptMessage is used in v1 and v3 - allowing the decrypt in v3 to also decrypt ciphertexts using v1.

Hence, this change is safe to make. But there likely will need to be explicit logic to do this:

For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key. 

in the application code.

NODE_OPTIONS: '--max-old-space-size=8192'
- name: Submit test coverage to Coveralls
uses: coverallsapp/[email protected]
- name: Submit test coverage to Coveralls-next

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: curious - why was this updated due to the previous coveralls failing?

could it be due to v1.1.2 which is outdated - our formsg app is using v2. perhaps using v2 to match the main formsg app would be easier to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standardization sounds good! Reverted

Copy link
Contributor Author

@scottheng96 scottheng96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a good look at it!

Copy link

@kevin9foong kevin9foong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last thing before we merge this change!

}

/**
* Note on verifiedContent decryption for cryptoV3:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Note on verifiedContent decryption for cryptoV3:
* Note on verifiedContent decryption for cryptoV3:
* Although decryption is supported, verifiedContent encryption is not supported
* in cryptoV3 encrypt.
* This is to keep the encryption of verifiedContent and encryptedContent similar to storage mode - where
* verifiedContent and encryptedContent are defined and encrypted separately.

"@types/node": "^18.18.9",
"@typescript-eslint/eslint-plugin": "^4.25.0",
"auto-changelog": "^2.4.0",
"coveralls": "^3.1.1",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Thanks for identifying and removing this! 🎉 Always happy to remove stuff -> less things to maintain!

Copy link

@kevin9foong kevin9foong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@scottheng96 scottheng96 merged commit add008c into develop Sep 30, 2025
6 checks passed
@KenLSM KenLSM deleted the feat/add-verifiedContent-to-v3 branch November 3, 2025 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants